Add simplistic exponential backoff retry #6
Conversation
Introducing options.delay and options.factor. Now, by default even without specifying options.delay, there's a default delay of 100ms between retries in case of at least one failure. I think it makes sense as a default behavior, and anyway users can override this if needed.
|
If the code suits your idea, I will add a few docs. |
| logger.debug(`waiting for ${delay} ms before next retry for ${options.uri}`); | ||
| resolve(fetchDataWithRetry(tryCount)); | ||
| }, delay); | ||
| }); |
There was a problem hiding this comment.
Consider multiplying the delay by the factor after the fetch.
On might expect the second request to happen 100 ms after the first request, but currently it's 100 ms * factor instead.
There was a problem hiding this comment.
@markstos nope, this is in the catch clause. So as I understand it, this is already on the first failure. I did have the same thought as you are, and agree waiting on the first download would be wasteful :-).
There was a problem hiding this comment.
Ah sorry, I didn't read enough code context. Well done.
While a few PRs are in flight, we can move forward without needing to go through an even heavier fork. void666/request-promise-retry#6
|
Heh, go home @TravisCI, you're drunk if you're saying my second commit doesn't build if the first did 😋. |
|
Build gets triggered for every push I presume. Will review and close the requests. Thanks @batmat |
|
I realized my code does not work wrt. exponential backoffs. Only delays are taken in account 🤦♂️. Additional commit on its way. |
|
@void666 does this PR seem correct to you? Anything else you would like me to add? |
|
@void666 gentle ping. Do you think you'll be able to take a look soon-ish? :-). Thanks |
|
Hey, sorry for the delay, I will push this ASAP.
…On Mon, Oct 1, 2018, 2:10 PM Baptiste Mathus ***@***.***> wrote:
@void666 <https://github.com/void666> gentle ping. Do you think you'll be
able to take a look soon-ish? :-). Thanks
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADWSwaRIfjvxK6RNlU326H7mcxufQPntks5ugdT8gaJpZM4WpFxk>
.
|
|
@void666 : Any chance we could get this merged soon? :) |
@batmat Just add this to the readme and it should be good. I don't totally understand how the factor will change over multiple retries but I think this change is important to get pushed soon as there are many use cases this would be helpful. |
|
@mmcguff will try to add it later, but if you can use "GitHub suggestions", I will be able to add this directly from the phone or so (going to be a bit afk starting from now until tomorrow included, public holiday here). |
|
I recommend the the |
Updated the verbose, and resolved the conflicts. |
Introducing options.delay and options.factor.
Now, by default even without specifying options.delay, there's a default
delay of 100ms between retries in case of at least one failure.
I think it makes sense as a default behavior, and anyway users can
override this if needed.